gh-109543: Remove unnecessary hasattr check#109544
gh-109543: Remove unnecessary hasattr check#109544JelleZijlstra merged 2 commits intopython:mainfrom
Conversation
Also added a new test case covering the scenario I thought this might be about.
| class TD2(TD1): | ||
| b: str | ||
|
|
||
| self.assertIs(TD2.__total__, True) |
There was a problem hiding this comment.
I know you're not changing behaviour here, but are the precise semantics here specified anywhere? Mypy has some interesting behaviour here -- it treats all the keys present in TD2 as required, but all the keys present in TD1 as not required. That implies to me that TD2 should not be seen as a "total" TypedDict.
https://mypy-play.net/?mypy=latest&python=3.11&gist=e7ef88dd8c2b5697297c4739d471ac45
The __total__ attribute seems to be pretty meaningless these days, though, in a post-PEP-655 world:
>>> from typing import *
>>> class Foo(TypedDict):
... x: Required[int]
... y: NotRequired[str]
...
>>> Foo.__total__
TrueMaybe we should treat that as a bug and make it meaningful. Or maybe we should just be clearer in the documentation that __total__ doesn't indicate whether or not all keys in the TypedDict are required or not, it just literally indicates whether that specific TypedDict was constructed using total=True (but I'm not sure why that would be useful to anybody).
There was a problem hiding this comment.
The mypy behavior is right; that was the way to mix Required and NotRequired keys before PEP 655.
Agree that __total__ isn't that useful and you should generally look at __required_keys__ and __optional_keys__. However, I think the current behavior is right. If you do want to use __total__ for introspection, you should look not just at the current class's attribute, but also those of base classes.
There was a problem hiding this comment.
It's strange to me that we iterate through the base classes as part of the class construction in order to make sure we have accurate __required_keys__, __optional_keys__ and __annotations__ attributes on the produced class, but we don't do the same for the __total__ attribute. (We probably shouldn't be doing that for the __annotations__ attribute, since that's not the way __annotations__ works on any other class... but we do it anyway.) It seems very inconsistent to me.
There was a problem hiding this comment.
Submitted #109547 with some enhancements to the docs.
AlexWaygood
left a comment
There was a problem hiding this comment.
The semantics of this attribute seem pretty confusing -- I'm honestly wondering whether it might not be better to deprecate it. But I'm pretty confident you're correct that there's no way for __total__ to already be set at this point.
As discussed in comments to python#109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead.
|
|
|
|
|
|
As discussed in comments to #109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead.
As discussed in comments to pythonGH-109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead. (cherry picked from commit f49958c) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
As discussed in comments to pythonGH-109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead. (cherry picked from commit f49958c) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
… (#109983) As discussed in comments to GH-109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead. (cherry picked from commit f49958c) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Also added a new test case covering the scenario I thought this might be about.
As discussed in comments to python#109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead.
… (#109982) Enhance TypedDict docs around required/optional keys (GH-109547) As discussed in comments to GH-109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead. (cherry picked from commit f49958c) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Also added a new test case covering the scenario I thought this might be about.
As discussed in comments to python#109544, the semantics of this attribute are somewhat confusing. Add a note explaining its limitations and steering users towards __required_keys__ and __optional_keys__ instead.
In relation to python#109544 which changed this behavior. Signed-off-by: Daniel Sperber <github.blurry@9ox.net>
In relation to python#109544 which changed this behavior. Signed-off-by: Daniel Sperber <github.blurry@9ox.net>
In relation to python#109544 which changed this behavior. Signed-off-by: Daniel Sperber <github.blurry@9ox.net>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
…re is an assignment in the class body. (pythonGH-130460) In relation to pythonGH-109544 which changed this behavior. (cherry picked from commit d8ce092) Co-authored-by: Daraan <github.blurry@9ox.net> Signed-off-by: Daniel Sperber <github.blurry@9ox.net>
…hen there is an assignment in the class body. (GH-130460) (#130462) Add test checking value of a TypedDict's __total__ attribute when there is an assignment in the class body. (GH-130460) In relation to GH-109544 which changed this behavior. (cherry picked from commit d8ce092) Signed-off-by: Daniel Sperber <github.blurry@9ox.net> Co-authored-by: Daraan <github.blurry@9ox.net>
Also added a new test case covering the scenario I thought this
might be about.